Fix #1780, RTEMS CFE_FT_Global build failure #1781

astrogeco · 2021-08-09T20:55:00Z

Describe the contribution

Fix RTEMS CFE_FT_Global build failure #1780

Testing performed
cFE CI and bundle CI
See succesful bundle tests at https://github.com/astrogeco/cFS/runs/3284780164

Expected behavior changes
No behavior change, fixes build failure on RTEMS 4.11 and 5.0

System(s) tested on
GitHub Actions CI

Additional context
Fixed as part of IC:2021-08-10

Third party code
None

Contributor Info - All information REQUIRED for consideration of pull request
@astrogeco
@nmullane

astrogeco · 2021-08-09T21:00:02Z

@nmullane here's the PR for our fix

skliper · 2021-08-09T21:36:39Z

Minor nit - commit message looks like it's missing a verb. Preference would be "Fix #1780, Resolve RTEMS CFE_FT_Global build failure ..."

skliper · 2021-08-09T21:39:14Z

modules/cfe_testcase/src/cfe_test.c

+    /* Constant Table information used by all table tests */
+    CFE_FT_Global.TblName           = "TestTable";
+    CFE_FT_Global.RegisteredTblName = "CFE_TEST_APP.TestTable";
+    CFE_FT_Global.TblFilename       = "test_tbl.tbl";


If this is just used by cfe_test_table.c it's probably more appropriate to initialize there.

It's used by multiple functions across multiple source files. The alternative is to re-initialize in each one of them or initialize them in the first one but that introduces a test execution order dependency which feels too brittle.

In that case I'm good as-is.

Specifically

TBLContentAccessTestSetup(); TBLContentMangTestSetup(); TBLInformationTestSetup(); TBLRegistrationTestSetup();

An alternative would be to create a new function say TBLTests_Setup that only contains this initialization.

I actually prefer the idea of making a "Setup" function here that does this init, then it can be passed as the setup function as part of the UtTest_Add for any test that uses this table struct. That being said, its OK as is, I wouldn't hold this up.

I don't think doing it as part of setup function would work out because there is already a setup function which is used by most but not all of the table tests to register a table. Also since CFE_FT_Global is already instantiated in cfe_test.c I think it would make the most sense to also perform the initialization of this data just the once either there or perhaps it could be moved into cfe_test_table.c.

Opened an issue for future work #1797

astrogeco · 2021-08-10T14:38:38Z

Minor nit - commit message looks like it's missing a verb. Preference would be "Fix #1780, Resolve RTEMS CFE_FT_Global build failure ..."

Nit on the nit, wouldn't "Fix" be the verb in this case?

skliper · 2021-08-10T14:43:30Z

Minor nit - commit message looks like it's missing a verb. Preference would be "Fix #1780, Resolve RTEMS CFE_FT_Global build failure ..."

Minor nit - commit message looks like it's missing a verb. Preference would be "Fix #1780, Resolve RTEMS CFE_FT_Global build failure ..."

Nit on the nit, wouldn't "Fix" be the verb in this case?

Nope. You are "fixing" the #issue, by verbing the subject. To be hones even my suggested update isn't that great... much more helpful to state what you actually did, as in "Fix #1780, Removed multiple instantiations of CFE_FT_Global to resolve RTEMS build issue". Even better:

Fix #1780, Resolve RTEMS CFE_FT_Global build falure

 - Removed multiple instantiations of CFE_FT_Global
 - Moved initialization of CFE_FT_Global table info to main test routine
 - Swapped order of source files in build

jphickey · 2021-08-10T17:55:03Z

Not intending to start any tribal wars, but I tend to side with @astrogeco on this one - The "Fix" verb should be sufficient. That is, if you simply take out the issue number and said "Fix RTEMS CFE_FT_Global build failure" - that still makes sense. The #issue is just an additional descriptor in this case, to indicate what issue was fixed (and adhere to our pattern).

As space within the first line of an commit log is precious, I don't want to expend space on two verbs that basically mean the same thing ("Fix" and "Resolve"). Keep that for the long description part. In some ways its like saying "ATM Machine" (another pet peeve of mine).

That's my 2c at least - I don't really care either way, aside from the general requirement to keep the first line as short as possible (under 50 chars is ideal) so it works well with various tools/displays that use the first line of a commit message.

jphickey · 2021-08-10T17:58:15Z

modules/cfe_testcase/src/cfe_test.c

+    /* Constant Table information used by all table tests */
+    CFE_FT_Global.TblName           = "TestTable";
+    CFE_FT_Global.RegisteredTblName = "CFE_TEST_APP.TestTable";
+    CFE_FT_Global.TblFilename       = "test_tbl.tbl";


I actually prefer the idea of making a "Setup" function here that does this init, then it can be passed as the setup function as part of the UtTest_Add for any test that uses this table struct. That being said, its OK as is, I wouldn't hold this up.

skliper · 2021-08-10T19:00:59Z

Not intending to start any tribal wars, but I tend to side with @astrogeco on this one - The "Fix" verb should be sufficient. That is, if you simply take out the issue number and said "Fix RTEMS CFE_FT_Global build failure" - that still makes sense. The #issue is just an additional descriptor in this case, to indicate what issue was fixed (and adhere to our pattern).

As space within the first line of an commit log is precious, I don't want to expend space on two verbs that basically mean the same thing ("Fix" and "Resolve"). Keep that for the long description part. In some ways its like saying "ATM Machine" (another pet peeve of mine).

That's my 2c at least - I don't really care either way, aside from the general requirement to keep the first line as short as possible (under 50 chars is ideal) so it works well with various tools/displays that use the first line of a commit message.

What I'm really stuck on is the commit is just restating the issue... I definitely agree saying "Fix #xxx, Fixed issue blah" (or any other synonym of "fix") isn't useful. I agree my initial suggestion doesn't solve that. What I'd much rather see is a summary of the changes.

- Removed multiple instantiations of CFE_FT_Global - Moves the assignment of the TBL related elements of `CFE_FT_Global` from 'cfe_test_table' to `cfe_test` to avoid duplicate definitions error - Makes `cfe_test.c` first in CMakeLists dependency list Co-authored-by: Niall Mullane <nmullane@users.noreply.github.com>

- nasa/cFE#1781, RTEMS CFE_FT_Global build failure - nasa/cFE#1796, replace VOIDCALL assert macro

**Combines** - nasa/cFE#1772, v6.8.0-rc1+dev844 - nasa/osal#1127, v5.1.0-rc1+dev590 **Includes** *cFE* - nasa/cFE#1737, Move global count into test global struct. - nasa/cFE#1722, Add ES application control API functional tests - nasa/cFE#1743, Update coverage test to use UtAssert macros - nasa/cFE#1734, Add table api functional tests - nasa/cFE#1753, Add Generic Counter API test - nasa/cFE#1766, finish ES misc API functional test - nasa/cFE#1764, last char truncated in coverage log output - nasa/cFE#1728, Mistakes in some copyright headers - nasa/cFE#1767, Add misc time api functional test cFE - nasa/cFE#1749, Add Functional Test for EVS Reset Filters API - nasa/cFE#1781, RTEMS CFE_FT_Global build failure - nasa/cFE#1796, replace VOIDCALL assert macro *osal* - nasa/osal#1117, Add absolute branch coverage check

**Combines** - nasa/cFE#1772, v6.8.0-rc1+dev844 - nasa/osal#1127, v5.1.0-rc1+dev590 **Includes** *cFE* - nasa/cFE#1737, Move global count into test global struct. - nasa/cFE#1722, Add ES application control API functional tests - nasa/cFE#1743, Update coverage test to use UtAssert macros - nasa/cFE#1734, Add table api functional tests - nasa/cFE#1753, Add Generic Counter API test - nasa/cFE#1766, finish ES misc API functional test - nasa/cFE#1764, last char truncated in coverage log output - nasa/cFE#1728, Mistakes in some copyright headers - nasa/cFE#1767, Add misc time api functional test cFE - nasa/cFE#1749, Add Functional Test for EVS Reset Filters API - nasa/cFE#1781, RTEMS CFE_FT_Global build failure - nasa/cFE#1796, replace VOIDCALL assert macro *osal* - nasa/osal#1117, Add absolute branch coverage check Co-authored-by: Joseph Hickey <jphickey@users.noreply.github.com> Co-authored-by: Alex Campbell <zanzaben@users.noreply.github.com> Co-authored-by: Niall Mullane <nmullane@users.noreply.github.com> Co-authored-by: Paul <pavll@users.noreply.github.com>

astrogeco requested review from zanzaben and jphickey August 9, 2021 20:55

astrogeco changed the base branch from main to integration-candidate August 9, 2021 20:55

astrogeco force-pushed the 1780-RTEMS-build-failure branch from 1f461fe to 7a8202c Compare August 9, 2021 20:58

astrogeco added a commit to astrogeco/cFS that referenced this pull request Aug 9, 2021

Add nasa/cFE#1781, RTEMS fix

300c969

zanzaben approved these changes Aug 9, 2021

View reviewed changes

astrogeco changed the title ~~Fix #1780, RTEMS build failure~~ Fix #1780, RTEMS CFE_FT_Global build failure Aug 9, 2021

astrogeco changed the title ~~Fix #1780, RTEMS CFE_FT_Global build failure~~ Fix #1780, RTEMS CFE_FT_Global build failure Aug 9, 2021

skliper reviewed Aug 9, 2021

View reviewed changes

jphickey approved these changes Aug 10, 2021

View reviewed changes

astrogeco force-pushed the 1780-RTEMS-build-failure branch from 7a8202c to 8fca2b1 Compare August 10, 2021 18:46

astrogeco mentioned this pull request Aug 10, 2021

Make global test table setup in functional tests reusable #1797

Open

astrogeco force-pushed the 1780-RTEMS-build-failure branch from 8fca2b1 to 20fbd6f Compare August 10, 2021 19:04

astrogeco merged commit c16b7ae into nasa:integration-candidate Aug 10, 2021

This was referenced Aug 10, 2021

cFE Integration candidate: 2021-08-10 #1772

Merged

cFS-Bundle Integration Candidate:2021-08-10 nasa/cFS#333

Merged

astrogeco added a commit to nasa/cFS that referenced this pull request Aug 10, 2021

IC:2021-08-10, Add cFE PRs

eaf960b

- nasa/cFE#1781, RTEMS CFE_FT_Global build failure - nasa/cFE#1796, replace VOIDCALL assert macro

astrogeco added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Aug 11, 2021

astrogeco removed the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Aug 11, 2021

astrogeco deleted the 1780-RTEMS-build-failure branch August 20, 2021 23:50

skliper added this to the 7.0.0 milestone Sep 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #1780, RTEMS CFE_FT_Global build failure #1781

Fix #1780, RTEMS CFE_FT_Global build failure #1781

astrogeco commented Aug 9, 2021 •

edited

Loading

astrogeco commented Aug 9, 2021

skliper commented Aug 9, 2021

skliper Aug 9, 2021

astrogeco Aug 9, 2021

skliper Aug 9, 2021

astrogeco Aug 9, 2021 •

edited

Loading

jphickey Aug 10, 2021

nmullane Aug 10, 2021 •

edited

Loading

astrogeco Aug 10, 2021

astrogeco commented Aug 10, 2021

skliper commented Aug 10, 2021 •

edited

Loading

jphickey commented Aug 10, 2021

jphickey Aug 10, 2021

skliper commented Aug 10, 2021

Fix #1780, RTEMS CFE_FT_Global build failure #1781

Fix #1780, RTEMS CFE_FT_Global build failure #1781

Conversation

astrogeco commented Aug 9, 2021 • edited Loading

astrogeco commented Aug 9, 2021

skliper commented Aug 9, 2021

skliper Aug 9, 2021

Choose a reason for hiding this comment

astrogeco Aug 9, 2021

Choose a reason for hiding this comment

skliper Aug 9, 2021

Choose a reason for hiding this comment

astrogeco Aug 9, 2021 • edited Loading

Choose a reason for hiding this comment

jphickey Aug 10, 2021

Choose a reason for hiding this comment

nmullane Aug 10, 2021 • edited Loading

Choose a reason for hiding this comment

astrogeco Aug 10, 2021

Choose a reason for hiding this comment

astrogeco commented Aug 10, 2021

skliper commented Aug 10, 2021 • edited Loading

jphickey commented Aug 10, 2021

jphickey Aug 10, 2021

Choose a reason for hiding this comment

skliper commented Aug 10, 2021

astrogeco commented Aug 9, 2021 •

edited

Loading

astrogeco Aug 9, 2021 •

edited

Loading

nmullane Aug 10, 2021 •

edited

Loading

skliper commented Aug 10, 2021 •

edited

Loading